feat(web): add attachment staging lifecycle - #388
Conversation
|
CI context: both Linux Node jobs reach the pre-existing packed standalone Web CLI smoke, install the tarball, and then the server exits before announcing its origin ( This PR does not change package metadata, CLI startup, or peer-dependency resolution. Its local check, focused tests, full Node suite, and Vitest suite pass as recorded in the PR body. |
tt-a1i
left a comment
tt-a1i
left a comment
There was a problem hiding this comment.
Exact-head re-review of 9ebd185. The previous metadata bounds and missing cleanup findings have been addressed; focused tests pass (5/5). However, the new startup reclamation can delete a different live store and its unconsumed attachments. This is a lifecycle/data-loss blocker, independently reproduced with the real module in a unique temporary parent. No host/UI or deployed impact is claimed; the store is not yet wired to an upload route.
f8ac1bd to
eba69a6
Compare
|
@tt-a1i 已补齐 TTL、name/mime 256 UTF-8 bytes 限制及回归测试；随后又补上 owner/liveness proof，避免启动回收删除仍存活的 store。 |
|
@tt-a1i 已修复 P1：startup reclaim 现在读取 sibling PID owner marker；owner 存活则保留，旧无 marker 根目录才按 TTL 回收。live-store/orphan 回归通过。 |
tt-a1i
left a comment
tt-a1i
left a comment
There was a problem hiding this comment.
Exact-head review of be9eab7. The staging design has useful binding and cleanup boundaries, but two independently reproduced P2 resource-lifecycle problems remain. Please fix these in this PR and add regression coverage; no author code was modified during review. This module is not yet connected to an upload route, so these are component findings rather than claims about deployed behavior. Related tracking: #441.
| this.assertOpen(); | ||
| validateBinding(binding); | ||
| const totalBytes = this.validatePayloads(payloads); | ||
| if (this.stagedBytes + totalBytes > this.limits.maxStagedBytes) { |
There was a problem hiding this comment.
[P2] Bound active stages independently of payload bytes
Zero-byte payloads are accepted by validatePayloads, so this byte-only store admission never advances for them. Each successful stage still creates a directory/file and retains a Map record, while maxSettledReceipts only bounds settled records. On this exact head, a store configured with maxAttachments=1, maxAttachmentBytes=1, maxTotalBytes=1, maxStagedBytes=1, maxSettledReceipts=1 accepted ten separate zero-byte batches and retained ten directories; repetition has no configured bound. Add an active-stage/count or charged-overhead bound (or reject empty payloads if that is the intended contract), with a regression proving repeated submissions eventually reject without leaking partial staging artifacts.
| if (this.disposePromise) return this.disposePromise; | ||
| this.closing = true; | ||
| this.disposePromise = this.exclusive(async () => { | ||
| await rm(this.directory, { recursive: true, force: true }); |
There was a problem hiding this comment.
[P2] Remove the sibling owner marker during normal disposal
create writes ${directory}.owner beside the root, but dispose only removes the root. Reproduction on this head: create a store under a fresh temporary parent, stage a batch, await dispose, and readdir(parent) still returns .openpi-web-attachments-<id>.owner. Startup reclamation only removes markers together with matching directories, so orphan marker files are never reclaimed and accumulate across normal create/dispose cycles, increasing future startup scans. Dispose the owned marker as part of the same cleanup lifecycle and test that a fresh dedicated parent is empty after repeated create/dispose cycles.
2 participants
Problem
The attachment work tracked by #344 has admission metadata validation in review, but no host-owned lifecycle for canonical temporary bytes. Browser filenames must not become filesystem authority, and staged bytes need exact command ownership and deterministic cleanup.
Value
Future composer and multimodal prompt work gets a fail-closed storage primitive with bounded resource use, single-consume semantics, and explicit stale or settled receipts.
Approach
Add a private attachment staging store that:
This is a focused backend slice and intentionally does not duplicate #365/#380 metadata admission validation or #384 UI work.
Validation
Impact
Related to #344.